Labels inside node markers #128
Conversation
|
I've re-run CI on the last commit. One refimage needs updating. This PR unintentionally breaks 7 more refimages. For debugging I added the last commit. It fixes those regressions, but breaks the feature of that PR. I don't understand why. |
|
The reftests are just normal package tests, so if you run |
|
I think I discovered the problem. Line 230 in 791014b
|
Co-authored-by: Hans Würfel <git@wuerfel.io>
| - `ilabels=nothing`: `Vector` with label for each node | ||
| - `ilabels_color=labels_theme.color` | ||
| - `ilabels_fontsize=labels_theme.fontsize` | ||
| - `ilabels_attr=(;)`: List of kw arguments which gets passed to the `text` command |
There was a problem hiding this comment.
Perhaps we should note that ilabels_attr cannot be set interactively, and users should manipulate the underlying text plot directly!
There was a problem hiding this comment.
What do you mean by that? The idea behind this was basically that users can forward own observables into the underlying plot function without explicitly exposing each and every attribute of the scatter/line/text plots in the graphplots recipe
There was a problem hiding this comment.
g = complete_graph(3)
visible = Observable(false)
graphplot(g; node_attr=(;visible))
visible[] = true #interactive update
There was a problem hiding this comment.
That would be the same as nlabels_attr, elabels_attr, node_attr, edge_attr, arrow_attr, right?
Probably this issue should be discussed separately?
There was a problem hiding this comment.
@asinghvi17 do you mean that _, _, p = graphplot(...), p[:ilabel_attr] = ... doesn't work?
There was a problem hiding this comment.
_, _, p = graphplot(...), p[:ilabel_attr] = ...doesn't work
I did mean that - the usage which @hexaeder suggested should work fine. This wouldn't work because a reference to the attributes at the time of plot creation is splatted, and changing the attributes afterwards wouldn't have an effect.
I guess we should probably clarify that somewhere? It's not a blocker for the PR by any means, just something I noticed.
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## master #128 +/- ##
==========================================
- Coverage 80.35% 79.76% -0.60%
==========================================
Files 4 5 +1
Lines 667 687 +20
==========================================
+ Hits 536 548 +12
- Misses 131 139 +8
☔ View full report in Codecov by Sentry. |
|
@hexaeder this is ready for your review. I wasn't sure if that feature should be shown in the feature walk-through instead of (or in addition to) the reftests. |
Closes #57
Test and docs are missing, will follow soon.